[TRTLLMINF-191][infra] Use native pytest capture for S3 logs - #16739
[TRTLLMINF-191][infra] Use native pytest capture for S3 logs#16739niukuo wants to merge 5 commits into
Conversation
|
/bot run --disable-fail-fast |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe S3 pytest integration now transforms captured report sections, inlines or spools output, supports synchronous and deferred uploads, and drains pending spool files. Jenkins and integration runners enable FD capture and perform cleanup. Unit tests cover transformation, recovery, reruns, registration, and xdist behavior. S3 deferred output workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Jenkins
participant Pytest
participant UploadLogPlugin
participant Spool
participant S3
Jenkins->>Pytest: enable capture=fd and deferred S3 upload
Pytest->>UploadLogPlugin: process pytest report sections
UploadLogPlugin->>Spool: write oversized captured output
UploadLogPlugin->>S3: upload or schedule deferred upload
Jenkins->>Spool: drain pending uploads after pytest
Spool->>S3: upload orphaned files
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #60995 [ run ] triggered by Bot. Commit: |
|
PR_Github #60995 [ run ] completed with state
|
|
PR_Github #61007 [ run ] triggered by Bot. Commit: |
|
PR_Github #61007 [ run ] completed with state
|
|
PR_Github #61180 [ run ] triggered by Bot. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_common/s3_output.py`:
- Around line 268-274: Update the hashlib.md5 call in normalize_test_name to
pass usedforsecurity=False, preserving the existing nodeid-derived suffix and
filename normalization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 67cdefca-2930-4fea-beb8-6027f9e62363
📒 Files selected for processing (7)
jenkins/L0_Test.groovyjenkins/scripts/slurm_run.shtests/integration/defs/test_unittests.pytests/test_common/s3_output.pytests/test_common/s3_output_hooks.pytests/unittest/test_s3_output.pytests/unittest/tools/test_test_to_stage_mapping.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/defs/test_unittests.py
- jenkins/L0_Test.groovy
- tests/test_common/s3_output_hooks.py
|
PR_Github #61248 [ run ] triggered by Bot. Commit: |
|
PR_Github #61180 [ run ] completed with state |
|
PR_Github #61248 [ run ] completed with state
|
|
PR_Github #61338 [ run ] triggered by Bot. Commit: |
|
PR_Github #61338 [ run ] completed with state
|
|
PR_Github #61449 [ run ] triggered by Bot. Commit: |
|
PR_Github #61449 [ run ] completed with state
|
|
PR_Github #61495 [ run ] triggered by Bot. Commit: |
|
PR_Github #61495 [ run ] completed with state
|
|
PR_Github #61540 [ run ] triggered by Bot. Commit: |
|
PR_Github #61540 [ run ] completed with state |
|
PR_Github #62381 [ run ] triggered by Bot. Commit: |
|
PR_Github #62381 [ run ] completed with state
|
|
PR_Github #62658 [ run ] triggered by Bot. Commit: |
|
PR_Github #62658 [ run ] completed with state
|
|
PR_Github #62710 [ run ] triggered by Bot. Commit: |
|
PR_Github #62710 [ run ] completed with state
|
|
PR_Github #62807 [ run ] triggered by Bot. Commit: |
|
PR_Github #62807 [ run ] completed with state
|
|
Reviewed the whole change. Moving off the bespoke session-capture machinery onto native pytest capture is the right call, and the 1.
testFilter[(DETAILED_LOG)] ? "-s" : "",and appends On 2. Output from a test that never finishes is now lost rather than already in the console log. The plugin only observes output at
Worth saying explicitly in the description whether that trade was intended. 3.
Minor: removing the The Not blocking on 3 or the minor. 1 is a working knob that stops working on the default path; 2 is a diagnosability trade that should at least be a stated decision. |
|
Addressed the actionable points in
One clarification on the before/after behavior: S3-enabled
|
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
|
PR_Github #64295 [ run ] triggered by Bot. Commit: |
|
PR_Github #64295 [ run ] completed with state
|
|
PR_Github #64461 [ run ] triggered by Bot. Commit: |
|
PR_Github #64461 [ run ] completed with state
|
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The core rewrite is sound and the unit coverage (phase merging, cumulative-section dedup, reruns, deferred backpressure, drain ownership) is convincing. The description accurately states the hard-kill tradeoff, and dropping test_s3_stdout_echo_requires_explicit_opt_in is consistent with the deleted groovy lines.
My concerns are concentrated on the recovery path — the drain exists for abruptly terminated pytest runs, but the wiring leaves it uncovered in the most likely termination scenarios:
- Direct CI: the drain runs after the rerun logic, which can rethrow and skip it (inline comment).
- Sbatch: the only drain is per-rank in
slurm_run.sh, which never runs on a walltime kill or stage-level scancel. The Slurm cleanup (rm -rf ${jobWorkspace}, L0_Test.groovy:742) doesn't know about the sibling spool dir, so orphaned.s3-spool-${jobUID}directories accumulate unboundedly in/home/svc_tensorrt/bloom/scripts/on every cluster. Please add spool-root removal tocleanupCommands(the path is derivable fromjobWorkspace), or drain from the login node before cleanup.
One placement decision worth documenting: the spool root is a sibling of --output-dir, so leftover spool files are also absent from results-*.tar.gz. Combined with the documented hard-kill limitation, a killed test's completed-phase output can now vanish with no artifact fallback at all if the drain also fails. If keeping the spool out of the results tarball is intentional, a sentence in the module docstring would save the next reader the archaeology.
| if (ENABLE_UPLOAD_TEST_RESULTS) { | ||
| sh """ | ||
| python3 ${llmSrc}/tests/test_common/s3_output.py \ | ||
| --drain-spool "${WORKSPACE}/${stageName}" || true |
There was a problem hiding this comment.
This drain is skipped on the crash paths it exists for. If the regular tests fail and rerunFailedTests crashes without producing a results file, it rethrows (L0_Test.groovy:3785) out of the catch above, and execution never reaches this block — exactly the scenario (pytest died mid-run) that leaves deferred spool files behind. runIsolatedTests throwing has the same effect. Suggest wrapping the test-execution section in try { ... } finally { <drain> } so recovery always runs.
| pytest_exit_code=$? | ||
| echo "Rank${SLURM_PROCID} Pytest finished execution with exit code $pytest_exit_code" | ||
| python3 "$llmSrcNode/tests/test_common/s3_output.py" \ | ||
| --drain-spool "$jobWorkspace" || true |
There was a problem hiding this comment.
This per-rank drain never runs when the Slurm job hits walltime or is scancel'd — the abrupt-termination case the spool is designed for. Since the spool root is a sibling of $jobWorkspace (_spool_root puts it at <parent>/.s3-spool-<name>), the existing cleanup rm -rf ${jobWorkspace} (L0_Test.groovy:742) doesn't remove it, so orphaned .s3-spool-${jobUID} dirs accumulate unboundedly in /home/svc_tensorrt/bloom/scripts/ on every cluster. Add spool-root removal to cleanupCommands, and consider a login-node drain there too so those logs are recovered rather than deleted.
| streams[stream_key] = stream | ||
| else: | ||
| if stream.finalized: | ||
| raise RuntimeError(f"Captured output arrived after {stream.filename} was finalized") |
There was a problem hiding this comment.
Raising here propagates out of pytest_runtest_logreport processing and becomes a pytest internal error, breaking reporting (and log upload) for the test. This plugin is log-shipping infrastructure — an unforeseen plugin interaction that delivers a late section should degrade, not fail the run. Suggest logger.warning(...) and either appending anyway or dropping the section.
| config = json.load(config_file) | ||
| owner_host = config.get("hostname") | ||
| owner_pid = int(config.get("pid", 0)) | ||
| if owner_host != current_host or (owner_pid and _process_is_alive(owner_pid)): |
There was a problem hiding this comment.
PID-reuse caveat: if the recorded pid has been recycled by an unrelated live process on the same host, the drain skips this spool — and for inner (nested-pytest) spools there is no later retry, so the logs are stranded and the dead report URLs stay dead. Probably acceptable given the short window between pytest exit and drain, but worth a comment; recording the process start time alongside the pid would close it if it ever bites.
| return False | ||
| finally: | ||
| if s3_output_module is not None: | ||
| s3_output_module.drain_pending_uploads( |
There was a problem hiding this comment.
drain_pending_uploads isn't fully exception-proof (e.g. _create_s3_client raises RuntimeError when boto3 is missing, and its per-config except doesn't cover that). An exception raised here in the finally replaces the function's return — turning the graceful return False failure report into a test error. Wrap this call in a try/except Exception: log since it's best-effort by design.
| def _resume_session_capture(self): | ||
| if self._session_capture is not None: | ||
| self._session_capture.resume_parent() | ||
| def _write_spool_file(self, test_name: str, filename: str, content: str) -> str: |
There was a problem hiding this comment.
_write_spool_file has no production callers — only the unit tests use it to fabricate leftover spool files (test_s3_output.py:469,493). Those tests could use _append_spool_file (identical except O_TRUNC vs O_APPEND on a fresh file); dropping this method removes ~15 duplicated lines.
7 participants
Description
The existing S3 plugin maintains its own session-level and per-test FD capture
machinery alongside pytest capture. This duplicates pytest lifecycle handling,
adds per-test capture overhead, and makes report sections and upload state harder
to keep consistent.
This PR switches S3 test logs to pytest's native
--capture=fdoutput andtransforms the resulting report sections:
one S3 URL per stream and attempt.
recoverable spool.
own ordering.
DETAILED_LOGkeeps its existing live-console purpose. When it is enabled, theJenkins command retains
-sand deliberately omits--capture=fdand theper-test S3 upload arguments; native FD capture and live console output cannot
both own the same descriptors.
Abrupt termination limitation
The plugin only receives native captured output after pytest creates a report
for a phase. If pytest is terminated with
os._exit, SIGKILL, or a stage-levelkill while a phase is still running, that phase produces no report and its
in-flight output cannot be transformed or recovered by
--drain-spool.This differs from the implementation currently on
main:-sthere does notsend S3-enabled test output directly to Jenkins because the early
SessionCaptureredirects FD 1/2 into named.s3-spoolfiles. A hard-killedtest is therefore also absent from the live console, but its raw session spool
can remain in
results-*.tar.gz. With native pytest capture, the underlyingtemporary capture file may disappear with the pytest process, so that artifact
fallback is no longer guaranteed. This is an explicit tradeoff of removing the
bespoke capture layer; live hard-kill diagnostics require a separate FD-level
tee design. Pytest's
tee-sysmode is not sufficient because it does not covernative FD writes, MPI workers, or subprocess output.
The previous
enableS3EchoStdoutparameter is also removed. It defaulted todisabled and depended on the legacy capture implementations removed here.
Test Coverage
tests/unittest/test_s3_output.pycovers inline output, stream merging,duplicate reports, reruns, failed-output tails, deferred uploads, and spool
recovery.
tests/integration/defs/test_unittests.pyexercises nested pytest capture andpending-upload draining.
pre-commit run --files jenkins/L0_Test.groovyPR Checklist